Make generated output files optional #46
Conversation
GeertJohan
force-pushed
the
configurable-output-files
branch
2 times, most recently
from
7f0f622 to
f0f8faa
Compare
| @@ -1,10 +0,0 @@ | |||
| -- ** Database generated with pgModeler (PostgreSQL Database Modeler). | |||
There was a problem hiding this comment.
The file still exists but its empty because there are no diffs.
GeertJohan
requested review from
Copilot and
provokateurin
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR makes generated output files (SQL, PNG, SVG) optional and configurable through the trek.yaml configuration file, addressing issue #37. When output types are defined in the config, files are generated at customizable or default paths. The initial migration logic has also been updated to generate diffs instead of directly copying SQL files.
Changes:
- Added
outputconfiguration section to control which file types (SQL, PNG, SVG) are generated - Refactored SQL export to use a temporary directory, only copying to output path if enabled
- Added SVG export capability and made PNG export conditional
Reviewed changes
Copilot reviewed 8 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/configuration/config.go |
Added Output, OutputFile types and GetOutputPath() method to support optional output files |
internal/templates/trek.yaml.tmpl |
Updated template to include empty output section for SQL and SVG by default |
internal/pgmodeler.go |
Renamed functions (consistent casing) and added PgmodelerExportSVG() function |
cmd/generate.go |
Modified generation logic to conditionally create output files and use temp directory for SQL |
tests/output/trek.yaml, example/trek.yaml |
Added output configuration sections |
tests/output/*.gen.{sql,svg}, example/*.gen.{sql,svg} |
New generated output files |
tests/output/migrations/001_init.up.sql, example/migrations/001_init.up.sql |
Updated migration files with different content structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GeertJohan
force-pushed
the
configurable-output-files
branch
from
f0f8faa to
6b4904e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GeertJohan
force-pushed
the
configurable-output-files
branch
from
6b4904e to
eaa31b8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: {{.}}{{end}} | ||
| output: | ||
| sql: {} | ||
| svg: {} |
There was a problem hiding this comment.
The trek.yaml template now includes output configuration for sql and svg but not png. Consider documenting this behavior or adding a comment in the template to explain that png can also be configured if needed. This would help users understand all available output options.
| svg: {} | |
| svg: {} | |
| # Note: PNG output can also be configured here if needed, for example: | |
| # png: {} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sqlContent, err = os.ReadFile(tmpSQLPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read sql file: %w", err) | ||
| } | ||
| err = os.WriteFile(filepath.Join(wd, sqlPath), sqlContent, 0o644) //nolint:gosec |
There was a problem hiding this comment.
Why not copy the file without read/write?
There was a problem hiding this comment.
How? There is no os.Copy right?
There was a problem hiding this comment.
io.Copy, but I guess in doesn't really matter given the amount of data we're handling here.
| sql: {} | ||
| svg: {} |
There was a problem hiding this comment.
For the example it would be nice to have one with an explicit path.
There was a problem hiding this comment.
I thought about that, but the example is generated via trek init which doesn't have env vars to set these.. I don't think adding env vars there makes sense. I think instead we should improve documentation in general. And/or maybe the example dir shouldn't be generated?
There was a problem hiding this comment.
Yeah maybe get rid of the example, since it's empty anyway it doesn't have that much value and it isn't even useful as a starting point, since we have trek init.
There was a problem hiding this comment.
Lets pick that up in a separate story/PR; to improve docs in general.
provokateurin
left a comment
provokateurin
left a comment
There was a problem hiding this comment.
Please regenerate the tests
GeertJohan
force-pushed
the
configurable-output-files
branch
from
eaa31b8 to
98fc879
Compare
|
Rebased. Also re-ran tests but it didn't make any changes. |
Shouldn't the config file change after? |
|
The config file already has the new outputs right? |
2 participants
Fixes #37